node: Consolidate borsch library usage#4892
Conversation
| errStr: "", | ||
| }, | ||
| { | ||
| name: "success -- trailing bytes are ignored", |
There was a problem hiding this comment.
This is a change in behaviour, right? Previously I believe this returned an error, and that's probably safer
There was a problem hiding this comment.
If gagliardetto doesn't do this then I think that's arguably a bug in that library
There was a problem hiding this comment.
Got this from an LLM FWIW:
_Borsh rejects trailing bytes. The reference implementation (borsh-rs) treats any unconsumed bytes after deserialization as an error.
The high-level entry points enforce this:
- from_slice / try_from_slice — after T::deserialize, check if !v_mut.is_empty() and return Err(InvalidData, "Not all bytes read") (borsh-rs/borsh/src/de/mod.rs).
- from_reader / try_from_reader — after deserializing, attempt to read 1 more byte; success (non-EOF) triggers the same "Not all bytes read" error.
The low-level deserialize / deserialize_reader methods themselves do not check — they only consume what the type needs. So the "no trailing bytes" rule is enforced at the API boundary, not per-field. The written spec (the README pseudocode) doesn't call it out explicitly, but the reference implementation's behavior is the de facto standard, and the error constant ERROR_NOT_ALL_BYTES_READ makes the stance unambiguous: the entire input must be consumed._
There was a problem hiding this comment.
Yes definitely a change of behaviour, although I couldn't think of a way to abuse this, and I also couldn't think of a neat way to keep the behaviour without just creating a wrapper function with the extra trailing bytes validation step. Any strong opinions (including abandoning this PR)?
There was a problem hiding this comment.
I think the PR is good and I do think it would be better to make just such a wrapper function! I'd prefer that the Guardian follows the expected behaviour of Borsh even if it's not exploitable at the moment. It'll keep things easier to reason about IMO, plus set us up to be more compatible with other libraries.
It's also possible that the solana-go behaviour has been patched in a newer version, so if we upgrade that library in the future, that might address it also.
I think it'd be best if there was a code comment on top of it where we link the Borsh standard.
This would be easy to do in a follow-up PR instead, but it would mean that we'd have to revert these changes to the test behaviour later.
bemic
left a comment
bemic
left a comment
There was a problem hiding this comment.
I haven't really looked into "shim" code. Therefore, consider this comment more as an datapoint that you can take into account when making the decision.
I diff tested near/borsh-go v0.3.0 vs gagliardetto/binary v0.7.7 across all four affected struct layouts (PostMessageData, MessagePublicationAccount, ShimPostMessageData, ShimMessageEventData) using more than 2.5mil canonical, edge case and malformed inputs. No difference in accepted inputs or successfully decoded values, and no panic.
Only difference spot by gpt5.6:
The meaningful differences are that gagliardetto/binary decodes directly into the destination, so failed decoding may leave partial data and reused structs may retain an old slice when a zero-length payload is decoded. It also returns []byte slices that alias the input buffer, whereas near/borsh-go copied them; error messages and wrapping differ as well.
None of these differences currently breaks the reviewed flows because they use fresh destinations, discard values on error, and do not mutate decoded payloads.
|
@djb15 needs rebase |
3 participants
I spent some time looking through the existing tests and adding a couple of new ones to check whether we're hitting all the edge cases we care about. Not a place we want to be making a mistake, so I'm happy to abandon these changes if the risks outweigh the upside